Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom readiness queue #371

Merged
merged 1 commit into from
May 31, 2016
Merged

Custom readiness queue #371

merged 1 commit into from
May 31, 2016

Conversation

carllerche
Copy link
Member

WIP, not ready for merging. This PR will be the first step towards #360

interest: Cell<EventSet>,

// Poll opts
opts: Cell<PollOpt>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally pretty nervous about the direction this abstraction is headed in. As is it seems like it's incredibly difficult to get right in terms of preventing race conditions and other undefined behavior, and it's barely leveraging Rust types for managing this unsafety. Specifically this structure is conglomeration of:

  • Raw unsafe pointers
  • non-thread-safe Cell types
  • threadsafe atomics

This, combined with the fn inner_mut(&self) -> &mut ReadinessQueueInner signature below, make this very difficult to determine whether it's actually safe or not.

I think this would benefit quite a bit from extracting some of the unsafe components here to standalone and isolated features with safe interfaces, or very very clear unsafe interfaces with documentation as to what contracts need to be upheld. For example next_all_notes and prev_all_nodes looks a lot like a doubly linked list? Does the standard library's LinkedList not suffice here because atomics are necessary? Some other reason?

Similarly if there's a set of fields that are only accessed by a Poll on each node, then this may benefit from a more concrete interface to indicate such. For example I would expect something like:

struct PollFields { ... }

struct ReadinessNode {
    poll_fields: UnsafeCell<PollFields>,
}

impl ReadinessNode {
    fn poll_fields<'a>(&'a self, _proof: &'a Poll) -> &'a PollFields { /* ... */ }
}

Basically something like that where the API itself is very difficult to get wrong if properly used. That should help anyone else working around here or perhaps any future refactorings as well.

@carllerche
Copy link
Member Author

Looks like CI is green. I'm hoping to merge this!

if 0 == self.inner.pending.fetch_add(1, Ordering::Acquire) {
// Toggle readiness to readable
if let Some(set_readiness) = self.inner.set_readiness.as_ref() {
// TODO: Don't ignore result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO is handled, right?

@rrichardson
Copy link
Contributor

LGTM

impl<T> StdSender<T> {
pub fn send(&self, t: T) -> Result<(), SendError<T>> {
match *self {
StdSender::Bounded(ref tx) => tx.send(t).map_err(SendError::from),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be using try_send?

This commit adds the ability to implement `Evented` and use it with a `Poll`
instance as expected. This works by having a new readiness queue that sits
next to the OS specific selector.

Future work includes:

* Implementing Timer using the new queue
* Migrate Windows support to use the custom queue
* Have `Poll::poll()` take `&self`
* More documentation
@carllerche carllerche merged commit 6ca6326 into master May 31, 2016
@carllerche carllerche changed the title WIP - Custom readiness queue Custom readiness queue May 31, 2016
@carllerche carllerche deleted the custom-readiness-queue branch May 31, 2016 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants